Changes:

    cp_functions.php:
      + xoops_remove_file_quietly($path, $context): new helper for
        best-effort file removal. Skips non-existent paths, suppresses
        the unlink warning via a SCOPED error_reporting() toggle (no @
        operator) wrapped in try/finally, and re-checks file_exists()
        after a failed unlink — only logging when the file is still
        present so TOCTOU races resolve silently. Uses
        xoops_file_label() for non-sensitive path labels in warnings.
      - 9 @Unlink cleanup sites replaced with the helper.
      - @chmod($tempFile, $perms) replaced with checked chmod() that
        logs on failure but continues (content is already written).

    modules/system/class/maintenance.php:
      + Explicit `require_once .../include/cp_functions.php` so
        SystemMaintenance is self-sufficient regardless of which
        caller (admin, install, modulesadmin, preferences) loads it.
      - 6 atomic-write @Unlink cleanup sites replaced with the helper.
      - @chmod replaced with checked + warn (mirrors cp_functions.php).
      - cleanOrphanedAvatars(): @Unlink replaced with the helper plus
        a path-traversal-safe resolution. Avatar rows store
        'avatars/<filename>' (kernel/avatar.php and 14 admin/profile
        writers), so basename() would have stripped the directory and
        silently bypassed all orphan cleanup. The new form:
          - normalises backslashes to '/' (Windows-historic data)
          - strips leading slashes (defends against absolute paths)
          - resolves via realpath() so '../' segments collapse
          - confirms containment under realpath(XOOPS_UPLOAD_PATH)
            with a trailing-separator boundary check
          - confirms is_file() before removal.
        The DB row cleanup runs unconditionally (file gone or not).

  Intentionally retained:
    - The 7 @rename(...) calls inside `if (!...)` checks. These are
      the core atomic-move operations; the boolean return is already
      detected and reported via trigger_error(). Removing the @ would
      let PHP's native warning fire alongside our diagnostic, double-
      reporting one event into display_errors output. Comment block
      in each file documents the rationale.

…out of loop

  Address PR XOOPS#52 follow-up review (Copilot non-blocking comments):

  modules/system/class/maintenance.php (CleanAvatar):
    realpath(XOOPS_UPLOAD_PATH) and the trailing-separator prefix are
    constant for the whole run, so computing them per row in the
    while-loop is wasted filesystem work on installations with large
    orphaned-avatar tables. Hoisted outside the loop into \$uploadRoot
    and \$uploadRootPrefix; the in-loop check now uses str_starts_with
    against the cached prefix.

  tests/unit/htdocs/modules/system/SystemMaintenanceTest.php:
    Added six #[Test] cases covering the path-traversal-safe orphan
    cleanup logic introduced when @Unlink() was replaced by
    xoops_remove_file_quietly():

      - cleanAvatarRemovesValidAvatarFileUnderUploadRoot — happy path
      - cleanAvatarSkipsTraversalPathButStillDeletesDbRow — ../ blocked
      - cleanAvatarSkipsAbsolutePathButStillDeletesDbRow — /etc/hosts
      - cleanAvatarHandlesMissingFileAndStillDeletesDbRow
      - cleanAvatarHandlesEmptyAvatarFileAndStillDeletesDbRow
      - cleanAvatarNormalisesBackslashesInAvatarFile

    Each test asserts exec() is called exactly twice (avatar DELETE +
    avatar_user_link cleanup) regardless of file removal outcome, so
    the 'DB rows deleted regardless of file outcome' invariant is
    codified rather than relying on inspection.

    Filesystem fixtures live in a unique scratch subdirectory under
    XOOPS_UPLOAD_PATH/avatars/_test_<pid>_<uniqid> with finally-block
    cleanup so the tests do not collide with each other or with
    anything else in the upload tree.

  CI run XOOPS#52 failed with 6 IncompatibleReturnValueException errors:
    Method exec may not return value of type int, its declared return type is \"bool\"

  XoopsMySQLDatabase::exec(string \$sql): bool returns true on success
  and false on failure (or false from PHP's mysqli_query for many DDL
  statements). The CleanAvatar() tests stubbed exec() with willReturn(1),
  which works under loose return-type checking but trips PHPUnit 11's
  strict return-type validation.

  Fix: all 6 exec() stubs use willReturn(true).

…st; (int) avatar_id

  Address PR XOOPS#52 follow-up review (Copilot, CodeRabbit Trivial):

  modules/system/class/maintenance.php (CleanAvatar):
    Cast $myrow['avatar_id'] to int once at the top of the loop body
    and use $avatarId in both DELETE statements (avatar row + the
    empty-avatar_file fast path). Defence-in-depth on the SQL concat
    even though the value is DB-origin; also silences SonarCloud's
    string-concatenation warning on these statements per project SQL
    hygiene.

  tests/unit/htdocs/modules/system/SystemMaintenanceTest.php:

    cleanAvatarSkipsTraversalPathButStillDeletesDbRow:
      The previous form created the fixture in sys_get_temp_dir() but
      used '../../<basename>' as the avatar_file. That string resolves
      from htdocs/uploads/ to a path under/near the project root —
      NOT the temp dir. realpath() returned false, the cleanup was
      skipped, and the test passed for the wrong reason: it never
      exercised the containment-prefix branch.

      New form: place the fixture at dirname(realpath(XOOPS_UPLOAD_PATH))
      so '../<basename>' actually resolves to it. realpath() now
      succeeds, and the prefix check is the rejection mechanism.
      Added an explicit assertSame() sanity assertion comparing the
      resolved path to realpath($outside) so the test self-documents
      that it really exercises the containment branch.

    cleanAvatarSkipsAbsolutePathButStillDeletesDbRow:
      Replaced the hard-coded /etc/hosts assertion with a temp fixture
      via tempnam(). Windows CI runners don't have /etc/hosts at the
      same path, and even on POSIX systems the test relied on a
      pre-existing system file. tempnam() works on every supported OS.

    stubAvatarSweep():
      Removed the misleading "Returns the mock for further expectations"
      docblock line. The method is `: void`; callers attach further
      expectations directly to the $db they passed in.

  Address PR XOOPS#52 follow-up review (Copilot, 3 actionable):

  cp_functions.php:
    + xoops_chmod_quietly($path, $perms, $context): new helper
      mirroring xoops_remove_file_quietly(). Suppresses chmod()'s
      native PHP warning via the same scoped error_reporting() toggle
      + try/finally and emits a single project-standard trigger_error
      on the boolean false return. Without this, a single chmod
      failure produced TWO log lines (PHP warning + trigger_error).
    - The chmod() + trigger_error block in xoops_write_file_atomically()
      replaced with the helper.

  modules/system/class/maintenance.php:
    - chmod() + trigger_error block in writeFileWithWarning() replaced
      with the same helper (already require_once'ing cp_functions.php).
    - CleanAvatar(): containment check narrowed from
      realpath(XOOPS_UPLOAD_PATH) to realpath(XOOPS_UPLOAD_PATH .
      '/avatars'). Custom avatars are stored as 'avatars/<filename>'
      (kernel/avatar.php and 14 admin/edituser writers), so the broad
      upload-root check would have allowed deletion of any file under
      uploads/ if avatar_file pointed elsewhere — legacy data, custom-
      module write, or accidental insertion. The narrow prefix is
      defence-in-depth: skip rather than silently delete unrelated
      uploads. Hoisted out of the loop so the realpath() resolution
      is computed once per sweep.

  tests/unit/htdocs/modules/system/SystemMaintenanceTest.php:
    + cleanAvatarSkipsNonAvatarsSubdirUnderUploadRoot: places a
      fixture at uploads/files/<unique>.doc and sets avatar_file to
      'files/<unique>.doc'. Asserts the resolved path is under
      uploads/ (proving the test exercises the narrow-prefix branch,
      not the realpath-fail branch), runs CleanAvatar(), and verifies
      the fixture survives — codifying the narrowing as a regression
      test.

  No @ operators remain in either file's unlink/chmod paths; the
  seven @rename(...) calls inside `if (!...)` checks are still
  retained with the documented rationale.

…atar

  Address PR XOOPS#52 follow-up review (Copilot, line 316):

  placeFixtureAvatar() and the three inline file_put_contents() sites
  in cleanAvatar* tests previously ignored the return value. If the
  write failed (FS permissions, full disk, etc.) the happy-path tests
  could pass for the wrong reason — CleanAvatar() would find no file
  to delete and assertFileDoesNotExist() would succeed even though
  the cleanup never ran on a real fixture.

  All four fixture-creation sites now:
    - capture file_put_contents() return into $bytesWritten
    - assertNotFalse with a path-suffixed message for triage
    - assertSame(strlen(content), $bytesWritten) so partial writes
      are caught too, not just false-return failures

  Sites covered:
    - placeFixtureAvatar() (the one Copilot flagged)
    - cleanAvatarSkipsTraversalPathButStillDeletesDbRow
    - cleanAvatarSkipsAbsolutePathButStillDeletesDbRow
    - cleanAvatarSkipsNonAvatarsSubdirUnderUploadRoot

  Address PR XOOPS#52 follow-up review (CodeRabbit, 3 findings):

  cp_functions.php (xoops_chmod_quietly, xoops_remove_file_quietly):
    Initialise $ok = false before each try block. error_reporting(0)
    does NOT disable user-defined error handlers; only the native
    warning. A handler that throws (e.g. a strict ErrorException
    conversion that doesn't check error_reporting() & $errno) would
    propagate out of the try block before chmod()/unlink() returns,
    leaving $ok unset and triggering an "Undefined variable" warning
    on the subsequent if (!$ok) check. The defensive default makes
    the helpers safe under any error-handler shape.

  modules/system/class/maintenance.php (CleanAvatar):
    Drop the unused `$result1 = ` capture on both $this->db->exec()
    calls. PHPMD flagged this; the value was never read so the
    assignment was misleading (no error propagation either way).
    Removing makes the discard explicit.

    Also added `@throws \RuntimeException` to the CleanAvatar() docblock
    per project convention — the method does throw RuntimeException
    when the avatar query fails, but the PHPDoc was silent about it.

  Address PR XOOPS#52 follow-up review (Copilot, 2 findings — CodeRabbit APPROVED):

  modules/system/class/maintenance.php:
    Add a `defined('XOOPS_ROOT_PATH') || exit()` guard before the
    require_once. The previous code commented out the guard but
    immediately uses XOOPS_ROOT_PATH in require_once, so direct
    access (or any include before bootstrap) would fail with an
    undefined-constant fatal that leaks server path detail in the
    error message. The guard makes direct access a clean exit.

  include/cp_functions.php (xoops_remove_file_quietly):
    file_exists() returns false for broken symlinks, so a dangling
    symlink would skip both the early-return guard AND the
    post-unlink existence check — the cleanup helper would never
    attempt removal and never warn that it was left behind. Add
    `|| is_link($path)` to both checks: unlink() can remove broken
    symlinks just fine, and the targets they point to are not what
    this helper is responsible for.

… guard

  Address PR XOOPS#52 follow-up review (Copilot, 4 findings):

  modules/system/class/maintenance.php:

    Direct-access guard (line 24):
      Switched from `if (!defined(...)) { exit(); }` to the project-
      standard one-liner:
        defined('XOOPS_ROOT_PATH') || exit('Restricted access');
      Consistent with the other guards across the codebase, and the
      'Restricted access' message is informative for log triage.

    CleanAvatar (symlink safety, lines 232-241):
      Previous code called realpath() on the full avatar path, then
      used the resolved path for both containment validation AND for
      the unlink. That's unsafe: realpath() follows symlinks, so an
      avatar entry that's a symlink within uploads/avatars/ would
      resolve to its target and the unlink would delete the wrong
      file (potentially another avatar) while leaving the symlink in
      place.

      New form keeps two paths:
        - $avatarCandidate = original path (used for is_file/is_link
          and the unlink itself — operates on the symlink, not the
          target)
        - $avatarParent    = realpath(dirname($candidate))
          (used ONLY for containment validation — resolves the
          parent directory's location without resolving the symlink
          itself)

      is_file() also gained an `|| is_link()` so symlinks within the
      avatars/ subtree are now eligible for cleanup.

      Smoke-tested: a symlink at uploads/avatars/<link> pointing to
      uploads/avatars/<real> is now correctly cleaned up (the
      symlink is removed, the target survives).

    PHPDoc (line 167):
      @return boolean → @return bool for consistency with the rest of
      the file.

… writeFileWithWarning

  modules/system/class/maintenance.php:

    CleanAvatar() — return contract:
      The method's @return bool was always returning true regardless
      of whether any DELETE actually succeeded. If a row delete or
      the final avatar_user_link cleanup failed, the orphaned row
      was left behind and the caller had no way to know.

      Track a $deleteOk flag across all three exec() sites
      (empty-avatar_file branch, regular avatar-row branch, final
      avatar_user_link cleanup) and return it. Existing CleanAvatar
      tests stub exec() with willReturn(true) so $deleteOk stays
      true and the return value is unchanged in the happy path.

    writeFileWithWarning() — restore observability:
      When the rename($tempFile, $filename) replace step fails but
      the rename($backupFile, $filename) restore step succeeds, the
      log previously said only "Failed to replace guard file: <X>",
      leaving operators unsure whether manual recovery was needed.

      Track $restoredBackup and fold the status into the existing
      composite failure warning:
        "Failed to replace guard file: <X> (original restored)"
      when the restore worked, or the unsuffixed form (plus the
      pre-existing "Failed to restore original guard file" line)
      when both steps failed and manual intervention is needed.
      No additional success warnings introduced.

…ll-byte paths

  xoops_chmod_quietly() / xoops_remove_file_quietly(): wrap the fs syscall
  (and, in the remove helper, the file_exists()/is_link() probes) in
  catch(\Throwable) so a ValueError from a "\0"-bearing path — or a
  throwing user error handler — cannot abort the caller's unrelated work.

  CleanAvatar(): gate dirname/realpath/is_file/is_link/remove on
  "\0" not in normalized avatar_file; collapse the empty-string special
  branch into the same condition. Per-row and avatar_user_link DELETEs
  still run unconditionally so a malformed path never blocks reclaiming
  the orphaned row.

  Tests: add cleanAvatarHandlesNullByteAvatarFileAndStillDeletesDbRow,
  covering the ValueError-skipped path with both DELETEs asserted.